refactor(dev): run the dev server's ordinary I/O on one session-scoped platform runtime (Effect FileSystem phase 2, PR 2) - #551
Conversation
…scoped platform runtime (phase 2, PR 2)
…latform.ts (installer bundle stays byte-identical)
…; platformRunOf resolves the edge (public declaration graph stays free of effect)
🦋 Changeset detectedLatest commit: 581163b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
commit: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a6d39cc7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /** The dev server's session runtime; absent, each program runs on its own `platformLayer`. */ | ||
| readonly platformRuntime?: DevPlatformRuntime; |
There was a problem hiding this comment.
Keep the session runtime out of public service options
EvalServiceOptions is exported through agent-bundle/api, but this new structurally typed option cannot be supplied correctly by package consumers: platformRunOf accepts only object identities registered by the internal createDevPlatformRuntime factory and throws TypeError for every other conforming { close(): Promise<void> } value, while neither the factory nor runtime type is exported from a package entry. Thus consumer code can type-check and then fail in the EvalService constructor. Keep this plumbing in an internal options type, or expose and document a supported runtime API.
AGENTS.md reference: AGENTS.md:L71-L77
Useful? React with 👍 / 👎.
Phase 2 of Effect
FileSystem/Pathadoption, second PR: the dev server. Behavior-preserving; follows the contract indocs/effect-conventions.md(FileSystem/Path section, keep-raw list binding). First PR: #540.What changed
Layer wiring.
startDevServer(src/dev/workbench-server.ts) creates onemakeScopedEffectRuntime(platformLayer)per session viacreateDevPlatformRuntime()(src/dev/platform-run.ts) — inside the function, never at module top level (effectis a CLI cold-start cost, #530) — and releases it from the returned session'scloseaftersession.close()has closed every service that ran on it. A failed start closes the runtime and reports both errors throughDevServerStartErrorif the cleanup also fails.Every dev service takes the runtime as an optional
platformRuntime?: DevPlatformRuntimeconstructor option.DevPlatformRuntime(src/dev/platform-runtime.ts) is a deliberately Effect-free handle (close()only): service option types sit on the package's public declaration graph, whichpublic-api.test.ts("keeps every public declaration graph free of effect") forbids from importingeffect. Implementations resolve the handle to itsPlatformRunedge withplatformRunOf(options.platformRuntime)—PlatformErrorunwrapped to the Node cause exactly likerunWithPlatform; absent a handle it isrunWithPlatform, so every service stays constructible on its own (and every existing service test keeps passing unchanged). Both modules live underdev/, not insrc/effect/platform.ts, which is bundled into the emitted installers (kept byte-identical, see below).Migrated to
FileSystemon the session runtime (ordinary reads, temp directories, removals):dev/project-service.ts— config identity read (readFileBytes)dev/package-build-service.ts— stale output-file removal (fs.remove(..., { force: true }))dev/host-install-manager.ts— MCP config document read (readFileString,ENOENTviaisPlatformErrno) and writes (writeFileString)dev/skill-document-service.ts— document body readsdev/workbench-assets.ts— rootrealPath, assetstat+ read; missing root/asset stays a missdev/runtime-provider-loader.ts— providerrealPathcontainment +stat(resolveDevRuntimeProvidertakes the handle as its fourth argument)dev/eval/eval-service.ts,dev/playground/{hook,host-discovery,mcp-probe,native,script}-playground-service.ts— reads,makeTempDirectory,removedev/runtime-generation-store.ts— manifest/asset reads andmakeDirectorythroughFileSystem, but onrunWithPlatform: providers construct the store through the publiccreateRuntimeGenerationStorefactory whose effect-free options contract (runtime-store-contracts.ts, exported fromagent-bundle/api, feat(api): export the dev.runtime.provider protocol types, errors, store and registry contracts from agent-bundle/api #528) has no session runtime to hand it. Adding one would put a dev-server handle on a provider-facing contract for no caller.Two lifetimes made explicit (both preserved contracts):
dev/mcp-session/mcp-session-service.ts— the plugin-data directory is acquired into its own session-lifetimeScope(not awithTempDirectorybracket, since it outlives the call): the finalizer removes it;McpSession.close()closes that scope (releasePluginData), and when the open fails before a session exists the open path closes it instead.mcp-session-types.tscarries theplatformRuntimeoption.dev/playground/script-playground-service.ts— workspace creation (makeTempDirectory) and release (remove) stay separate steps so a removal failure lands in the result'scleanupFailures(workspace-release-failed) instead of replacing the script's outcome.Kept raw, with reason (file:line on this head):
dev/project-service.ts:2lstat/readdir/realpath— symlink identity andDirentwalk (keep-raw:lstat, link identity);readFileat :172 pairs with thelstatin the samePromise.allfor one consistent snapshot.dev/skill-document-service.ts:1lstat/readdir/realpath— same symlink-aware walk.dev/host-install-manager.ts:10lstatrows — per the task ("keep itslstatrows raw").dev/package-build-service.ts:183rmdir— prune-if-empty relies onENOTEMPTYsemantics;FileSystem.removehas no non-recursive "only if empty" form with the same error.dev/eval/eval-service.ts:2–3,dev/playground/native-playground-service.ts:1–2—O_NOFOLLOWopens,link,rename,lstat,FileHandle(durable/identity protocols on the keep-raw list).dev/playground/hook-playground-service.ts:1cp— injectable test seam (options.copy, documented at :104 and :393); the temp directory around it iswithTempDirectory.dev/playground/mcp-probe-service.ts:328rmwithmaxRetries/retryDelay— retrying removal of a directory a just-killed child may still hold;FileSystem.removehas no retry policy. The no-child paths useFileSystem.remove(removeUnusedPluginData).dev/playground/lifecycle-replay-service.ts:199existsSync— synchronous render-child path probe in the constructor path.dev/runtime-generation-store.ts:10lstat/open+sync/rename/writeFile(wx)/readdir— the durable staging→publish protocol (keep-raw: durable-fs).dev/watcher.ts(chokidar),dev/epoch-store.ts(durable protocol),dev/dev-lock.ts.Tests
tests/effect-filesystem-phase2-dev.test.ts(unit):layerNoopruntimes pin the call sequence forworkbench-assets(realPath→stat→readFile, containment miss, memoised second read) and the script playground (makeTempDirectorythenremoveas separate steps; aremovedefect surfaces ascleanupFailures: [{ code: 'workspace-release-failed' }]with the script's exit code intact); real temp dirs pin the OS semantics (missing root/asset → miss like the formerENOENTcatch; real workspace removed; MCP session plugin-data removed onclose, service-owned release runs once and its failure is reported fromclose);platformRunOfresolves a session runtime, returnsrunWithPlatformforundefined, rejects a foreign handle, and unwrapsPlatformErrorto{ code: 'ENOENT' }.pnpm typecheck0 errors;pnpm lintclean;pnpm test:unit229 files / 3316 passed.cli,public-api,dev-server,dev-workbench,dev-host-install,eval-service,eval-workbench,hook-playground-*,host-discovery-dev-server,host-install-session,lifecycle-replay-dev-server,mcp-probe-dev-server,mcp-session-*,native-playground-service,playground-*,runtime-generation-store,runtime-provider,script-playground-service,skill-document-service,workbench-surface*,emitted-artifact-effect-surface— 165 passed, 0 failed.cli.test.tsconfirms--version/--helpstill resolve noeffectmodule.pnpm test:examples:browser(examples-real.e2e, real Chrome, 1440×900, drivesstartDevServerfromsrc/): 5/5 — populated state,hooks-and-scriptsdiagnostic-stale → diagnostic-repaired,skills-startercapability-stale → capability-repaired,audiobook-curatorroutes-catalog-stale → routes-catalog-repaired.pnpm docs:site:buildgreen (language parity ok, 1812 pages).Timings
Same machine, same path, package built at each ref; medians.
origin/mainagent-bundle --version(15 runs)startDevServeronexamples/host-test, start → ready (3 runs)session.close()Dev-server startup is dominated by the initial project build; the ~6 ms runtime construction is invisible against it, and the before/after gap is machine-load noise (the "before" runs overlapped a
pnpm build), not a speedup claim.Artifacts
examples/host-test(90 files) andexamples/audiobook-curator(20 files) built at the same path fromorigin/mainand this head: byte-identical (diff -r). The installer bundlessrc/effect/platform.ts; the runtime edge deliberately lives indev/platform-run.tsso that module is unchanged.Docs / changeset
docs/effect-conventions.md: phase table row and the FileSystem/Path section describe the session runtime, the Effect-freeDevPlatformRuntimehandle +platformRunOfsplit and why (public declaration graph), theruntime-generation-storeexception, and the two explicit lifetimes. Nowebsite/page changes: no CLI flag, config key, public export, diagnostic, or host behavior changed. Changeset:.changeset/effect-filesystem-phase2-dev-server.md(patch).Self-review
Reviewer:
change-risk-reviewersubagent on GPT-5.6 (sol, high), read-only, against this branch vsorigin/main. No breaking-change, doc, or changeset findings. Two bug findings, one test finding:workbench-server.tsclose: an in-flight asset / Skill / host-discovery handler can reach the runtime afterplatformRuntime.close(). Dismissed as benign.foreground.close()(insidesession.close()) destroys every socket and awaitsserver.close()before the runtime is disposed, so any handler still running is writing to a destroyed response; its platform read now rejects (interrupted / disposed) instead of completing into a dead socket, and every handler rejection is already caught atforeground-server.ts:498and turned into a diagnostic write that is a no-op on a destroyed response. No unhandled rejection, no observable difference to a client, no resource left behind (the runtime's Scope owns nothing of the handler's).workbench-server.tsclose: thefinallylet a disposal failure replace the session's own close failure. Fixed: the session failure is rethrown as-is (disposal still runs, its rejection settled), and a disposal failure surfaces only after a clean session close.ManagedRuntime.dispose()rejecting, which theplatformLayerfinalizers cannot produce; the ordering itself (services close, then runtime) is covered by every dev-server integration suite closing a live session, andeffect-filesystem-phase2-dev.test.tscovers the runtime's own open/close and edge behavior.Per this work's no-PR-comments rule, review notes live here.